fix(plugins): qualify browse and filter queries with the table schema#1758
Conversation
datlechin
added
the
abi-additive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bda130dbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func fetchFilteredRowCount(table: String, filters: [(column: String, op: String, value: String)], logicMode: String) async throws -> Int? | ||
| // Statement generation (optional, for NoSQL plugins) | ||
| func generateStatements(table: String, columns: [String], primaryKeyColumns: [String], changes: [PluginRowChange], insertedRowData: [Int: [PluginCellValue]], deletedRowIndices: Set<Int>, insertedRowIndices: Set<Int>) -> [(statement: String, parameters: [PluginCellValue])]? | ||
| func generateStatements(table: String, schema: String?, columns: [String], primaryKeyColumns: [String], changes: [PluginRowChange], insertedRowData: [Int: [PluginCellValue]], deletedRowIndices: Set<Int>, insertedRowIndices: Set<Int>) -> [(statement: String, parameters: [PluginCellValue])]? |
There was a problem hiding this comment.
Adding this method to the public PluginDatabaseDriver protocol changes the protocol witness table. A driver bundle compiled against the previous PluginKit v18 cannot provide this witness, but PluginManager.currentPluginKitVersion/minimumCompatiblePluginKitVersion still accept v18 bundles, and DataChangeManager.generateSQL now calls this overload through any PluginDatabaseDriver; user-installed drivers built before this change can therefore pass validation and fail at runtime. Please bump the PluginKit version/min compatibility (and plugin Info.plists) or avoid changing the protocol ABI.
Useful? React with 👍 / 👎.
1 participant
Problem
Opening data for a SQL Server table or view outside the
dboschema failed with "Invalid object name 'X'" (#1754). Structure and View definition worked; only data browsing failed.Root cause
PluginDatabaseDriverhas schema-less andschema:-bearing overloads ofbuildBrowseQuery/buildFilteredQuery. The app calls theschema:overload with the correct schema, but PluginKit's default for that overload silently drops the schema and delegates to the schema-less one. MSSQL and Oracle implemented only the schema-less overload, so the schema never reached the FROM clause. MySQL/PostgreSQL avoid this by returning nil, which makes the app'sTableQueryBuilderqualify the table itself.Fix
Read path (the reported bug):
MSSQLSchemaQueries(TableProMSSQLCore): pure, testedqualifiedName/browse/filteredbuilders that own schema qualification, reusing the existingbracketedhelper.schema:overloads ofbuildBrowseQuery/buildFilteredQuery, qualifying the FROM target ([schema].[table]/"SCHEMA"."TABLE"). The schema-less overloads delegate withschema: nil, keeping the registration probe working.Write path (found in review, same root cause):
Before this, editing and saving a non-default-schema table generated
UPDATE [table]with no schema, which resolved against the login default schema (error, or a silent write to the wrong table). The read fix made those tables browsable and therefore editable, so this had to ship together.generateStatements(table:schema:...)protocol overload (default delegates to the schema-less one).DataChangeManagercarries the tab's schema and passes it through; the three table-context call sites supplytableContext.schemaName.FK navigation (found in review):
MainContentCoordinator+FKNavigationusedfkInfo.referencedSchemafor the first FK-filtered load while the tab metadata and later pages used the resolvedtargetSchema, so the first query could be unqualified and later pages could swap to a different table. Now usestargetSchemaconsistently.ABI
The new
generateStatements(table:schema:...)requirement has a default implementation, so this is additive and binary-compatible (nocurrentPluginKitVersionbump), same pattern as the existingbuildBrowseQuery(table:schema:)overload.scripts/check-pluginkit-abi.shshows only the added requirement. Labeledabi-additive.Tests
TableProMSSQLCoreTests: 6 new cases for schema-qualified vs unqualified browse/filter output (24 pass). The DML generators are private plugin methods that aren't reachable from the test target; their qualification routes through the unit-testedMSSQLSchemaQueries.qualifiedName.Shipping
MSSQL and Oracle are registry-only plugins, so this reaches users only after re-releasing both plugins.
Follow-ups (pre-existing, not worsened by this PR)
referencedSchemais always nil and cross-schema FK following still can't qualify perfectly.ExportDataSourceAdapter) qualifies the FROM target with the database name, conflating database vs schema.Fixes #1754
https://claude.ai/code/session_0198faM6VCrViRU4XwRoS1DC